Restore receipts after recent fix #4248
Conversation
Longarithm
force-pushed
the
restore-receipts
branch
from
5b10e61
to
c116d83
Compare
Longarithm
requested review from
bowenwang1996,
evgenykuzyakov,
frol,
MaksymZavershynskyi and
SkidanovAlex
as code owners
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
| // True iff the current block is the first one in the chain with the current version | ||
| pub is_first_block_of_version: bool, | ||
| // True iff the current block containing chunk for some specific shard is the first one in the | ||
| // chain with the current version |
There was a problem hiding this comment.
This seems to be only true if first epoch of the current version have block with chunk (and also currently only for shard 0)
| @@ -272,15 +282,29 @@ lazy_static_include::lazy_static_include_bytes! { | |||
| MAINNET_STORAGE_USAGE_DELTA => "res/storage_usage_delta.json", | |||
| } | |||
|
|
|||
| #[cfg(feature = "protocol_feature_restore_receipts_after_fix")] | |||
| lazy_static_include::lazy_static_include_bytes! { | |||
There was a problem hiding this comment.
Maybe swap this with next constant so that things related to single migration is consecutive
chain/chain/src/migrations.rs
Outdated
| runtime_adapter.get_epoch_protocol_version(&prev_epoch_id)?; | ||
| Ok(protocol_version != prev_epoch_protocol_version) | ||
| } | ||
| Err(_) => Ok(true), |
There was a problem hiding this comment.
This looks very suspicious to me. Have you accounted for all possible ways an attacker can maliciously send you some block that such get_prev_epoch_id_from_prev_block returns an error?
There was a problem hiding this comment.
I've tried to handle error coming from KeyValueRuntime.get_prev_epoch_id_from_prev_block but now found that my implementation was incorrect. Fixed
chain/chain/src/migrations.rs
Outdated
| Ok(chain_store.get_epoch_id_of_last_block_with_chunk(prev_block_hash, shard_id)? | ||
| != runtime_adapter.get_epoch_id_from_prev_block(prev_block_hash)?) |
There was a problem hiding this comment.
Please use separate variables to avoid very long expressions like this
|
@Longarithm @bowenwang1996 Will the restored receipts be stored in the storage (e.g. accessible via P.S. The context for this question is that we need to handle these on Indexer Framework side, and we recently bumped into local receipts that were delayed and had to hotfix the behavior in #4316. /cc @khorolets |
No. We only store receipts that are present in some chunk and here it is a special case. |
|
Result of applying receipts to current mainnet state: Commit used for testing: 6d2530b Logs: "Processed: 390, Delayed: 0" shows that all restored receipts were applied, and exit code 0 shows that no issues were happened. Some research of accounts balance. But receipts in considered block also affect this account. There are two such ones, and Same thing was checked for two previous blocks. |
This was one-off thing we needed a while ago. We no longer use it, so there's no need to support it. The code can always be restored from the git history. cc near#4248
This was one-off thing we needed a while ago. We no longer use it, so there's no need to support it. The code can always be restored from the git history. cc #4248
Currently rebased on #4274 to reuse
MigrationData.This PR has two goals:
Test plan
Check that
restored-receipts-verifier -> test_checking_differencesprocess_blocks -> test_restoring_receipts_mainnet